C#: Only use reachable feeds when private registries are configured#21385
C#: Only use reachable feeds when private registries are configured#21385
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the NuGet feed reachability checking logic in C# dependency fetching to support scenarios where customers only have access to private registries and not to public feeds like NuGet.org. The core change is to check both explicit and inherited feeds for reachability, and only pass reachable feeds to dotnet restore.
Changes:
- Refactored feed reachability checking to test both explicit feeds (from nuget.config files in the working directory and dependabot proxy) and inherited feeds (from nuget.config files outside the working directory)
- Modified
RestoreProjectsto receive only reachable feeds instead of all configured feeds - Extracted common feed reachability logic into a new
GetReachableNuGetFeedsmethod to reduce code duplication
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs
Show resolved
Hide resolved
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
I think this is a very good idea!
| { | ||
| logger.LogInfo("Checking that NuGet feeds are reachable..."); | ||
| // Exclude any feeds that are configured by the corresponding environment variable. | ||
| var excludedFeeds = GetExcludedFeeds(); |
There was a problem hiding this comment.
I think that the original intention with this list returned here is that they should be included in the feeds be used no matter whether the feed reachability check fails - this serves as an override mechanism.
| this.EmitUnreachableFeedsDiagnostics(allFeedsReachable); | ||
|
|
||
| var (initialTimeout, tryCount) = GetFeedRequestSettings(isFallback: false); | ||
| return (allFeedsReachable, reachableFeeds); |
There was a problem hiding this comment.
The reachableFeeds should also include the feeds that was execluded on l. 790.
|
|
||
| // If private package registries are configured for C#, then consider those | ||
| // in addition to the ones that are configured in `nuget.config` files. | ||
| this.dependabotProxy?.RegistryURLs.ForEach(url => explicitFeeds.Add(url)); |
There was a problem hiding this comment.
nit: Remove all the this prefixes when calling instance methods and accessing instance variables (this is the pattern used in the extractor).
| /// Populates dependencies with the relative paths to the assets files generated by the restore. | ||
| /// </summary> | ||
| /// <param name="projects">A list of paths to project files.</param> | ||
| private void RestoreProjects(IEnumerable<string> projects, HashSet<string>? configuredSources, out ConcurrentBag<DependencyContainer> dependencies) |
There was a problem hiding this comment.
Make rename configuredSources to reachableFeeds and add a comment to the method description, which communicates that
(1) If reachableFeeds are null then we haven't checked anything in relation to feed reachability.
(2) If reachableFeeds are not null then we should ONLY use these feeds.
| // `nuget.config` files instead of the command-line arguments. | ||
| string? extraArgs = null; | ||
|
|
||
| if (this.dependabotProxy is not null) |
There was a problem hiding this comment.
Do we need to modify this logic as well?
(1) If configuredSources are not null, then the registry feeds are already added to the configuredSources if they are reachable (maybe we should we just use configuredSources in this case?).
(2) The existing logic also looks a bit weird: configuredSources is only set in case nuget feed reachability is checked (due to the short-circuit logic on l. 122 in the original implementation). This means that if feed reachability is not checked then the private registry URLs completely override the nuget configuration. Is that the intended behavior - or did we expect allFeeds to be added in this case?
| } | ||
| } | ||
|
|
||
| using (var nuget = new NugetExeWrapper(fileProvider, legacyPackageDirectory, logger)) |
There was a problem hiding this comment.
The NugetExeWrapper also explicitly add the "default" NuGet feed in some cases. Maybe we should consider to add a feed reachability check (if checking nuget feed reponsiveness)?
| } | ||
|
|
||
| // Restore project dependencies with `dotnet restore`. | ||
| var restoredProjects = RestoreSolutions(out var container); |
There was a problem hiding this comment.
Should we also add the reachableFeeds logic for restoring solution files (and also make use of the private registries in this case)?
Some customers only allow access to their own private registries and do not allow access to e.g. the public NuGet feed. This PR refactors and modifies the
build-mode: nonelogic that tests whether feeds are reachable to also check the inherited feeds, and to only pass explicit and inherited feeds which are reachable todotnet restore.